Skip to content

Conversation

@w0nderfu11
Copy link
Contributor

@w0nderfu11 w0nderfu11 commented Aug 20, 2025

Extend the Entry Editor context menu to handle entries with multiple linked files. This improves UX when managing more than one file per entry and aligns behavior across single- and multi-file scenarios.

Changes:

  • Made general actions in StandardActions enum for multi-file items
  • Extract selection predicates into SelectionChecks and reuse for actions
  • Introduce SingleSelectionMenuBuilder and MultiSelectionMenuBuilder (pattern strategy fix)
  • Extend ContextMenuFactory to delegate to builder based on selection
  • Refactor ContextAction: unified enablement by bindings
  • Remove CopyMultipleFilesAction and merged into CopySingleFileAction
  • Wire LinkedFilesEditor to new ContextMenuFactory; keep double-click behavior
  • Update localization: add pluralized keys, remove obsolete ones; localization tests pass
  • Add/update unit tests: ContextActionTest, ContextMenuFactoryTest, SingleSelectionMenuBuilderTest,
    MultiSelectionMenuBuilderTest, SelectionChecksTest
  • Guard LinkedFileViewModel to avoid a JavaFX crash

Fixes #12567
Keywords: context menu, linked files, multi-selection, UI

Steps to test

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Checked documentation (developer & user docs): information is available and up to date. If not, I created an issue at https://github.com/JabRef/user-documentation/issues/ (or submitted a PR)
image

Extend the Entry Editor context menu to handle entries with multiple
linked files. This improves UX when managing more than one file per
entry and aligns behavior across single- and multi-file scenarios.

Changes:
- Add plural actions to StandardActions enum
- Rewrite ContextAction.execute() for multi-file cases
- Extend ContextMenuFactory to build multi-file items
- Rework MultiContextAction to operate on selections
- Introduce CopyMultipleFilesAction (new class)
- Update/add localization keys; tests pass
- Add unit tests: ContextActionTest, ContextMenuFactoryTest,
  MultiContextActionTest, CopyMultipleFilesActionTest
- Guard LinkedFileViewModel to avoid a JavaFX crash

Fixes JabRef#12567
Keywords: context menu, linked files, multi-selection, UI
@w0nderfu11
Copy link
Contributor Author

image i set up checkstyle with your guide but it doesn`t show me some problems in test classes, only in main module)

@w0nderfu11 w0nderfu11 marked this pull request as ready for review August 20, 2025 14:08
@w0nderfu11
Copy link
Contributor Author

Hi @koppor, all checks have passed ✅ Could you please review and approve when you have time?
Thanks a lot!

@subhramit
Copy link
Member

subhramit commented Aug 20, 2025

i set up checkstyle with your guide but it doesn`t show me some problems in test classes, only in main module)

image

Check this setting on the right (screenshot taken from here)

@w0nderfu11
Copy link
Contributor Author

i set up checkstyle with your guide but it doesn`t show me some problems in test classes, only in main module)

image Check this setting on the right (screenshot taken from [here](https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/intellij-13-code-style.html#put-jabrefs-checkstyle-configuration-in-place))

thank you anyway, but all check are done succesefully, so i wait for review

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR so far.
Some remarks made.

I noticed the Strategy Pattern was somewhat broken there (not by you). Please fix it.
grafik

@calixtus
Copy link
Member

I mean reuse existing commands with changed text: eg 'Download file(s)'

@w0nderfu11
Copy link
Contributor Author

Thx for your feedback, i will research the problems you marked and try to solve, if it`s possible, thx a lot

w0nderfu11 and others added 2 commits September 1, 2025 16:50
…sActionTest.java

Co-authored-by: Carl Christian Snethlage <[email protected]>
…menu

- Introduce ContextMenuBuilder + SingleSelectionMenuBuilder + MultiSelectionMenuBuilder
- Extract shared checks/openContainingFolders into SelectionChecks
- ContextMenuFactory delegates to strategies; no more branching by selection size
- LinkedFilesEditor initializes ContextMenuFactory once and just requests menus on right-click
- Replace plural menu commands with single StandardActions; multi-selection handled inside builders
- Fix NPE in ContextAction executable binding by removing null observables and binding menu disable state properly
- Remove obsolete MultiContextAction and its tests

Follow-ups:
- Convert hardcoded labels to i18n keys (Download file(s), Open folder(s), etc.)
- Consider removing *_FILES actions from enum if unused elsewhere
- Re-add unit tests around builders/factory (TestFX/JUnit5) once API stabilized
@w0nderfu11
Copy link
Contributor Author

w0nderfu11 commented Sep 2, 2025

Thanks for your PR so far. Some remarks made.

I noticed the Strategy Pattern was somewhat broken there (not by you). Please fix it. grafik

ContextMenuFactory now delegates to ContextMenuBuilder strategies (SingleSelectionMenuBuilder / MultiSelectionMenuBuilder) with shared checks in SelectionChecks.

@w0nderfu11
Copy link
Contributor Author

All the review has been done, i will test it manually and do unit test, after make a PR if all is OK

@calixtus
Copy link
Member

calixtus commented Oct 3, 2025

Hi, i think the next iteration will probably be the last one. I just made some quick refactorings to ContextMenuFactoryTest to make it a bit more readable. It was just like a wall that hit me will all the declarations in the single tests.

Please apply the suggested changes. After that, we will finish this PR and get it merged. Most is already good and you did a great job in this PR.

@w0nderfu11
Copy link
Contributor Author

Hi, @calixtus let`s make a summary and i will start work - so i fix thing above, and as @koppor said, split logic to package logic, right?

@calixtus
Copy link
Member

calixtus commented Oct 6, 2025

yes

@w0nderfu11
Copy link
Contributor Author

w0nderfu11 commented Oct 8, 2025

@koppor @calixtus @subhramit about merging to package ...logic. Would you mind if i create package in org,jabref.logic ---> or.jabref.logic.files

Other way - i found LinkedFileHandler in package org.jabref.logic.externalfiles

Or i even can make in this package additional file as LinkedFileCopier, for example

What do you prefer? I will do LinkedFileCopier in externalfiles package for now, after your answer we will see

image

@w0nderfu11 w0nderfu11 marked this pull request as draft October 8, 2025 11:28
@calixtus
Copy link
Member

calixtus commented Oct 8, 2025

Hey, we briefly talked about the refactoring, @koppor suggested. We think we should do this in a follow-up. So forget about moving stuff to logic and just polish this one. Then we finalize and merge.

@Siedlerchr
Copy link
Member

don't forget the openrewrite test

@w0nderfu11
Copy link
Contributor Author

don't forget the openrewrite test

Yes, sure, i will check submodules, do rewrite and changelog, ty for notifying

@w0nderfu11
Copy link
Contributor Author

What i did:

1.ContextMenuFactory - added jspecify NonNull and deleted static import, using Object now (Objects.requireNonNull(selection, "...")

  1. MultiSelectionMenuBuilder - merged all methods from SelectionChecks(it was deleted for non profit abstraction) and updated test class also. Function Copy to folder now allMatch (was anyMatch), so it available only for downloaded local files (if they are selected and exist)

  2. LinkedFilesEditor - returned old switch

  3. in CopyLinkedFilesAction - changed ErrorDialog to notification interface (was not split logic and gui as we discussed)

Now i will run all tests, test it in application finally and do commit soon!

@calixtus
Copy link
Member

You may have forgotten to push your commit.

@w0nderfu11
Copy link
Contributor Author

You may have forgotten to push your commit.

No-no, I`m sick now (ill), no energy for coding now, i will polish it soon do not worry

@calixtus
Copy link
Member

Oh ok, sorry. Get well soon!
No hurry.

@w0nderfu11
Copy link
Contributor Author

@calixtus @subhramit i have a problem
image

Gradle tries to find snapshot version but there is no one here)
I already reseted this file from my remote branch, tried from upstream but it`s not fixing

I mean i can just change version but i dont really wana push this file. Do you know whats the problem?

  • What went wrong:
    Could not determine the dependencies of task ':jabgui:test'.

Could not resolve all dependencies for configuration ':jabgui:testRuntimeClasspath'.
Could not find io.github.adr:e-adr:2.0.0-SNAPSHOT.
Searched in the following locations:
- https://repo.maven.apache.org/maven2/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://repo.maven.apache.org/maven2/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://oss.sonatype.org/content/repositories/snapshots/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://oss.sonatype.org/content/repositories/snapshots/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://jitpack.io/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://jitpack.io/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://oss.sonatype.org/content/groups/public/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://oss.sonatype.org/content/groups/public/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://maven-central.storage-download.googleapis.com/repos/central/data/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://maven-central.storage-download.googleapis.com/repos/central/data/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://sandec.jfrog.io/artifactory/repo/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://sandec.jfrog.io/artifactory/repo/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- file:/C:/Users/Vitaliy/jabref/jablib/lib/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- file:/C:/Users/Vitaliy/jabref/jablib/lib/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
Required by:
project ':jabgui'
project ':jabgui' > project :jablib
Could not find io.github.adr:e-adr:2.0.0-SNAPSHOT.
Searched in the following locations:
- https://repo.maven.apache.org/maven2/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://repo.maven.apache.org/maven2/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://oss.sonatype.org/content/repositories/snapshots/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://oss.sonatype.org/content/repositories/snapshots/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://jitpack.io/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://jitpack.io/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://oss.sonatype.org/content/groups/public/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://oss.sonatype.org/content/groups/public/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://maven-central.storage-download.googleapis.com/repos/central/data/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://maven-central.storage-download.googleapis.com/repos/central/data/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- https://sandec.jfrog.io/artifactory/repo/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- https://sandec.jfrog.io/artifactory/repo/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
- file:/C:/Users/Vitaliy/jabref/jablib/lib/io/github/adr/e-adr/2.0.0-SNAPSHOT/maven-metadata.xml
- file:/C:/Users/Vitaliy/jabref/jablib/lib/io/github/adr/e-adr/2.0.0-SNAPSHOT/e-adr-2.0.0-SNAPSHOT.pom
Required by:
project ':jabgui' > project :versions

@Siedlerchr
Copy link
Member

You need to merge in the latest main, then it will be resolved

@w0nderfu11
Copy link
Contributor Author

@calixtus Done

@w0nderfu11 w0nderfu11 marked this pull request as ready for review October 13, 2025 14:19
@w0nderfu11 w0nderfu11 requested a review from subhramit October 13, 2025 14:19
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@calixtus calixtus dismissed stale reviews from subhramit and koppor October 13, 2025 18:47

.

@calixtus calixtus added this pull request to the merge queue Oct 13, 2025
Merged via the queue into JabRef:main with commit a9b50c6 Oct 13, 2025
71 of 79 checks passed
@calixtus
Copy link
Member

Hooray!

@w0nderfu11 w0nderfu11 deleted the feat/multi-file-context-menu branch October 14, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: entry-editor dev: no-bot-comments If set, there should be no comments from our bots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Have the context menu of linked files working for multipe selected files.

5 participants